Convert SQLite JSON columns to JSONB#1224
Conversation
brandur
force-pushed
the
brandur-sqlite-jsonb-2
branch
from
d958f6b to
0df950c
Compare
Alright, so this one's been some cleanup that I've been intending for a while. Previously, sqlc made using JSONB in SQLite completely unusable because it sent back the binary format which you're explicitly not supposed to parse yourself. I ended up fixing this like a year ago in [1], but getting a sqlc release out the door took so long that I ended up forgetting about it. Here, modify our SQLite definitions so that JSON becomes JSONB, and add migration to convert existing installation values to the same. Luckily, this doesn't require a table rewrite because both JSON and JSONB are SQLite BLOB types. I didn't want to add a migration version just for SQLite, so to keep everything in sync I also added a version 7 for Postgres that's just a no-op with a comment. [1] sqlc-dev/sqlc#3968
brandur
force-pushed
the
brandur-sqlite-jsonb-2
branch
from
0df950c to
bfe5ba0
Compare
brandur
added
the
needs db migration
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
LGTM aside from the broader concern of no-op migrations for the vast majority of users.
| -- No-op migration to keep version numbers in sync across all drivers. | ||
| -- The SQLite driver uses this version for a JSONB conversion. |
There was a problem hiding this comment.
I hadn't considered this possibility before but tbh I don't love no-op migrations for the vast majority of users just to facilitate something in sqlite. We could consider pulling in some of these other items to make the v7 migration broadly useful everywhere? That would at least punt the issue for now. https://github.com/riverqueue/river/issues?q=is%3Aissue%20state%3Aopen%20label%3A%22needs%20db%20migration%22
There was a problem hiding this comment.
Yeah, I figured we'd probably combined this with #1115 at the very least (which contains fixes for all three items you linked).
I don't love the idea of shipping a DB migration that isn't totally necessary, but it's made a little more complicated by the fact that the longer we go without the migration, the more people will have to migrate later, so there is some cost to not doing this.
The good news with #1224 and #1115 is that we may be able to ship the migration, but make a clear note saying it's not a critical migration for Postgres, so if people want to defer it, they can. If there's ever another migration later on, they could always run it to get the cleanups at that point in time.
2 participants
Alright, so this one's been some cleanup that I've been intending for a
while. Previously, sqlc made using JSONB in SQLite completely unusable
because it sent back the binary format which you're explicitly not
supposed to parse yourself. I ended up fixing this like a year ago in
[1], but getting a sqlc release out the door took so long that I ended
up forgetting about it.
Here, modify our SQLite definitions so that JSON becomes JSONB, and add
migration to convert existing installation values to the same. Luckily,
this doesn't require a table rewrite because both JSON and JSONB are
SQLite BLOB types.
I didn't want to add a migration version just for SQLite, so to keep
everything in sync I also added a version 7 for Postgres that's just a
no-op with a comment.
[1] sqlc-dev/sqlc#3968